-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix verify logs repetition #344
base: main
Are you sure you want to change the base?
Fix verify logs repetition #344
Conversation
…earch) Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
@@ -103,7 +106,13 @@ func (s *MemorySource) LoadEnvelope(reference string, env dsse.Envelope) error { | |||
return nil | |||
} | |||
|
|||
func (s *MemorySource) Search(ctx context.Context, collectionName string, subjectDigests, attestations []string) ([]CollectionEnvelope, error) { | |||
func (s *MemorySource) Search(ctx context.Context, depth int, collectionName string, subjectDigests, attestations []string) ([]CollectionEnvelope, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikhailswift I found the issue which was producing the false negative results and it was the limiting of the memory source searches. I have now passed in the search dept integer to the Sourcer
interface, which feels slightly wrong as only the memory source search needs it. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the memory source needs the depth -- and I don't think we can actually skip over subsequent memory searches where depth > 0.
If we do one round of searches, and find some attestations that match, and others that don't, but then find some back ref subjects on one of the matched attestations, we may discover more that match on the next iteration of searches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current tests are passing even with this change, but I suspect it is not correct. Let me try to write a test that can demonstrate.
If you cherry-pick the two commits from #345 onto your branch, this does indeed break policy evaluation when we need to search for back refs. |
@mikhailswift thanks for review here. I will set as a requirement for this PR that we have an improvement to the current tests. |
Update: I am converting this to draft as @mikhailswift has highlighted some still existing issues with the code change. On top of that, additional tests should be added before this is ready for final review and merge. |
What this PR does / why we need it
Reopening this PR after #317 needed reverted due to issues with the attempts at limiting memory search invocations.
This PR introduces the same changes, but directly uses the search depth to decide whether to perform a search or not.
I am unsure whether changing the interface is the right way to go here, but figured I would raise the pull request for review anyhow.